feat: Add inspect metrics API #7197
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to f9d07e9 in 3 minutes and 22 seconds
More details
- Looked at
223lines of code in7files - Skipped
0files when reviewing. - Skipped posting
24drafted comments based on config settings.
1. pkg/query-service/app/summary.go:77
- Draft comment:
BUG: In ListMetrics, if SummaryService returns an error, RespondError is called with 'apiError' (from ParseSummaryListMetricsParams) instead of 'apiErr' returned by ListMetrics. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. pkg/query-service/app/summary.go:16
- Draft comment:
BEST PRACTICE: Check error from io.ReadAll instead of ignoring it. Consistently handle errors when reading r.Body. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. pkg/query-service/model/metrics_explorer/summary.go:19
- Draft comment:
TYPO: 'TimeSeriesTeeMap' is likely misspelled. Consider renaming to 'TimeSeriesTreeMap' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. pkg/query-service/model/metrics_explorer/summary.go:12
- Draft comment:
CLARIFICATION: Field name 'EndD' is used with JSON tag 'end'. Ensure this naming is intentional and consistent with API expectations. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. pkg/query-service/app/metricsexplorer/summary.go:159
- Draft comment:
PERFORMANCE: Converting dashboard data via JSON marshal and unmarshal may be inefficient. Consider type conversion if possible. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. pkg/query-service/app/summary.go:59
- Draft comment:
Adjust error log message to better reflect metric details retrieval error. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. pkg/query-service/app/summary.go:109
- Draft comment:
Clarify error log for related metrics parsing. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. pkg/query-service/app/summary.go:126
- Draft comment:
Update error log message for inspect metrics request. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states what was done without offering any guidance or questions.
9. pkg/query-service/app/metricsexplorer/summary.go:265
- Draft comment:
Document the rationale for the 70/30 weight split when combining similarity scores. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. pkg/query-service/model/metrics_explorer/summary.go:12
- Draft comment:
Rename field 'EndD' to 'End' for consistency with the JSON tag. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. pkg/query-service/app/clickhouseReader/reader.go:3898
- Draft comment:
Typo: "atleast" should be corrected to "at least". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. pkg/query-service/app/clickhouseReader/reader.go:5638
- Draft comment:
Typo: Variable 'atrributeValue' is misspelled; should be 'attributeValue'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. pkg/query-service/app/clickhouseReader/reader.go:3447
- Draft comment:
Typo: The function name 'CheckIfPrevousPaginateAndModifyOrder' should be renamed to 'CheckIfPreviousPaginateAndModifyOrder'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. pkg/query-service/app/clickhouseReader/reader.go:5320
- Draft comment:
Typo: 'ReleStateItem' seems to be a misspelling; consider renaming it to 'RuleStateItem' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. pkg/query-service/app/http_handler.go:4977
- Draft comment:
Typographical error: The comment on lines around 4977-4978 currently uses 'experssion' where it should be 'expressions'. Please correct it for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
16. pkg/query-service/app/http_handler.go:4960
- Draft comment:
Typographical consistency issue: The variable name 'errQuriesByName' appears multiple times. It appears to be a misspelling of 'errQueriesByName'. Consider fixing it for clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
17. pkg/query-service/app/metricsexplorer/parser.go:86
- Draft comment:
Consider rephrasing the error message 'time duration shouldn't be more than 30 mins' to something like 'Time duration should not exceed 30 minutes' for improved clarity and consistency with formal tone. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While the suggested wording is slightly more formal, this is a very minor change that doesn't affect functionality. The current message is already clear and understandable. The rules state not to make purely informative comments or obvious/unimportant changes. This seems to fall into that category - it's a stylistic preference rather than a necessary change.
The suggested wording is arguably more professional and consistent with formal API error messages. Error messages are part of the API contract and their clarity matters.
While formal error messages are good, the current message is already sufficiently clear and functional. This change would be purely cosmetic and doesn't warrant a review comment.
Delete this comment as it suggests a minor stylistic change that doesn't meaningfully improve the code or fix any actual issues.
18. pkg/query-service/app/metricsexplorer/summary.go:217
- Draft comment:
Typo detected in the case for TimeSeries treemap in the GetMetricsTreemap function: 'TimeSeriesTeeMap' should likely be 'TimeSeriesTreeMap'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
19. pkg/query-service/app/metricsexplorer/summary.go:172
- Draft comment:
Typographical inconsistency detected in the error type naming: 'UnMarshallingErr' is used. Consider changing it to 'UnmarshallingErr' for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
20. pkg/query-service/app/summary.go:98
- Draft comment:
Typo in the error message: 'error getting heatmap data' should likely be 'error getting treemap data' to match the context of the GetTreeMap function. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
21. pkg/query-service/app/summary.go:72
- Draft comment:
The error message on line 72 'error parsing metric list metric summary api request' appears redundant and awkwardly worded. Consider rephrasing it to something more clear, such as 'error parsing summary list metrics parameters'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
22. pkg/query-service/app/summary.go:132
- Draft comment:
In the GetInspectMetricsData function, the error message on line 132 reads 'error parsing metric query range params', which seems to be copy-pasted from another handler. It would be clearer and more accurate if it referenced 'inspect metrics' (e.g., 'error parsing inspect metrics parameters'). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While the comment is technically correct that the message could be more specific, this appears to be a consistent pattern throughout the codebase. The generic message is used in multiple places, suggesting it may be intentional. The change would be minor and wouldn't affect functionality. The current message still conveys the essential information - there was an error parsing parameters.
The consistent use of the generic message across handlers might be intentional for uniformity. Making this one message more specific while leaving others generic could actually reduce consistency.
Good point about consistency. While more specific messages might be nice, maintaining consistency across the codebase is probably more important here.
The comment should be deleted. The suggested change would break consistency with other handlers and the benefit would be minimal.
23. pkg/query-service/interfaces/interface.go:99
- Draft comment:
Typographical error: 'ReleStateItem' seems like it might be a typo for 'RuleStateItem'. Please verify the correct type name and update if necessary. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
24. pkg/query-service/model/metrics_explorer/summary.go:19
- Draft comment:
Typo detected: The constant 'TimeSeriesTeeMap' seems to have a typographical error. Consider renaming it to 'TimeSeriesTreeMap' for better clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_1m2aNpSqQtw0nrxu
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 74dd32c in 1 minute and 52 seconds
More details
- Looked at
22lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. pkg/query-service/app/metricsexplorer/summary.go:169
- Draft comment:
Consider checking for a nil or empty result if dashboards does not contain key metricName. The current code skips assignment if key is missing, but downstream consumers might rely on non-nil slice. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment is speculative - it suggests there "might" be downstream consumers that rely on non-nil slices, but provides no evidence. The current code's behavior of leaving the field unset when no dashboards exist seems reasonable. Without seeing the downstream consumers or struct definition, we can't verify if this is actually an issue.
I could be wrong about the nil slice being fine - perhaps there are important downstream consumers that do require non-nil slices. The current code could cause nil pointer exceptions.
While nil pointer issues are important, we should trust that the original author tested their changes. Without clear evidence of downstream requirements, this comment is too speculative.
Delete this comment as it is speculative and lacks evidence that nil slice handling is actually required by downstream code.
2. pkg/query-service/app/metricsexplorer/summary.go:169
- Draft comment:
Double JSON marshalling/unmarshalling might be unnecessary. Consider processing 'data' directly if possible to avoid performance overhead. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. pkg/query-service/app/metricsexplorer/summary.go:169
- Draft comment:
The variable 'dashboards' is now a map. Consider renaming it (e.g., to 'dashboardsMap') and adding a comment to clarify the expected JSON structure. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The variable name change would make the type more clear. However, the code is already quite readable - the type declaration makes it obvious it's a map, and the usage at line 175-176 makes the structure clear (metric name -> dashboard array). The current namedashboardsis not misleading since it still represents a collection of dashboards. Variable renaming suggestions should have a very strong justification.
The comment does point out a real opportunity for improved code clarity. The JSON structure is not immediately obvious without reading the usage code.
While valid, this is a minor stylistic suggestion rather than a significant issue. The code remains clear and maintainable with the current variable name.
Delete this comment as it suggests a minor stylistic change that doesn't significantly improve code clarity or correctness.
4. pkg/query-service/app/metricsexplorer/summary.go:175
- Draft comment:
If the key 'metricName' is not present in the dashboards map, the DTO field remains unassigned. Consider explicitly assigning an empty slice to avoid potential nil issues downstream. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
In Go, a nil slice is a valid zero value and is safe to use. Many Go functions and methods are designed to handle nil slices correctly. The comment suggests defensive programming, but it's not necessarily required or idiomatic in Go. Without seeing the downstream code that uses this DTO, we can't be certain that nil handling would be an issue.
I might be overlooking potential edge cases where nil slices could cause issues. Some JSON serializers or frontend code might handle nil differently than empty slices.
While there could be edge cases, in Go it's idiomatic to use nil slices. Without clear evidence of downstream issues, this suggestion is speculative and goes against Go conventions.
Delete this comment as it suggests a change that isn't clearly necessary and goes against Go idioms where nil slices are perfectly acceptable.
5. pkg/query-service/app/metricsexplorer/summary.go:219
- Draft comment:
Typo: In the case for TimeSeries metrics, 'TimeSeriesTeeMap' seems to be a misspelling of 'TimeSeriesTreeMap'. Please correct this to maintain consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. pkg/query-service/app/metricsexplorer/summary.go:393
- Draft comment:
Typo/Naming Issue: The helper function 'getQueryRangeForRelateMetricsList' appears to have a slight naming inconsistency. Consider renaming it to 'getQueryRangeForRelatedMetricsList' for improved clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_FCB7pvVEoB078VcJ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on ef2400b in 2 minutes and 9 seconds
More details
- Looked at
252lines of code in3files - Skipped
0files when reviewing. - Skipped posting
14drafted comments based on config settings.
1. pkg/query-service/interfaces/interface.go:16
- Draft comment:
The Reader interface is very large and covers a wide range of functionalities (metrics, logs, traces, TTL, dashboards, query progress, etc). Consider refactoring into smaller, more focused interfaces (e.g. MetricsReader, LogsReader, TracesReader) to better follow the Interface Segregation Principle and improve maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. pkg/query-service/app/metricsexplorer/summary.go:90
- Draft comment:
Using errgroup.WithContext to run concurrent queries is a good practice. Ensure that the error wrapping follows a consistent pattern (e.g. using errors or %w) so that errors are traceable. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. pkg/query-service/app/metricsexplorer/summary.go:155
- Draft comment:
In GetMetricsSummary, multiple concurrent goroutines are used to fetch metadata, samples, and alerts. Consider adding additional inline comments explaining the weighting logic if not obvious, and verifying that the combined error handling covers all cases. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. pkg/query-service/app/metricsexplorer/summary.go:322
- Draft comment:
When merging dashboards data, the code marshals and then unmarshals JSON. This double-conversion may be simplified by a direct conversion if possible. Consider refactoring for clarity and performance. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. pkg/query-service/interfaces/interface.go:133
- Draft comment:
The Reader interface exposes many query-related methods and some helpers for cache, query progress tracking, and even alert rules. It may be beneficial to split responsibilities into separate interfaces for better clarity and decoupling. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. pkg/query-service/app/metricsexplorer/summary.go:268
- Draft comment:
Clarify the rationale for using 0.7 weight for name similarity and 0.3 for attribute similarity when merging scores in GetRelatedMetrics. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. pkg/query-service/app/metricsexplorer/summary.go:503
- Draft comment:
Consider renaming 'validAttrs' to a more descriptive name (e.g., 'selectedResourceAttrs') in GetInspectMetrics for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The current name 'validAttrs' is actually quite clear - it stores attributes that have been validated to exist in both sets. The suggested name 'selectedResourceAttrs' doesn't add much more clarity since the selection criteria is more complex than just being resource attributes (they must be valid and then potentially limited to top 3). The current name focuses on the key aspect - these are "valid" attributes that passed our filtering criteria.
The suggested name does highlight that these are specifically resource attributes, which the current name doesn't convey. This could be relevant context.
While true, the resource attribute context is already clear from the surrounding code and comments. The key characteristic being captured in this variable is validity/filtering, not the resource nature.
The current name 'validAttrs' is appropriate and clear in context. The suggested rename would not meaningfully improve code clarity.
8. pkg/query-service/app/metricsexplorer/summary.go:430
- Draft comment:
Long functions like GetMetricsSummary and GetRelatedMetrics could be refactored into smaller helper functions to improve readability and maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. pkg/query-service/interfaces/interface.go:125
- Draft comment:
The Reader interface has grown very large; consider splitting it into focused interfaces (e.g., MetricsReader, TracesReader, LogsReader) for improved modularity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. pkg/query-service/interfaces/interface.go:55
- Draft comment:
Ensure error returns are consistent across methods. Some methods return *model.ApiError while others return error; standardizing these could improve clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. pkg/query-service/interfaces/interface.go:90
- Draft comment:
Consider adding documentation comments to the interface methods to describe their purpose, parameters, and expected behavior. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. pkg/query-service/app/clickhouseReader/reader.go:5638
- Draft comment:
Typo: The variable 'atrributeValue' is misspelled. Please change it to 'attributeValue' for clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. pkg/query-service/app/clickhouseReader/reader.go:5050
- Draft comment:
Typo: In log messages (e.g., 'closing go routine'), consider using the single word 'goroutine' as is conventional in Go. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. pkg/query-service/interfaces/interface.go:99
- Draft comment:
Typo alert: In the return type of GetOverallStateTransitions, 'ReleStateItem' appears to be a typo. Consider renaming it to 'RuleStateItem' for consistency with other naming in the file. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_rL79sH6fE4E84d8J
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 7d4cb88 in 55 seconds
More details
- Looked at
19lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. pkg/query-service/app/dashboards/model.go:617
- Draft comment:
Renamed JSON keys: 'widget_title' -> 'widget_name' and 'dashboard_title' -> 'dashboard_name'. Ensure that any existing API clients or documentation relying on the old key names have been updated to prevent backward compatibility issues. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to ensure that existing API clients or documentation are updated, which violates the rule against asking the author to ensure things are tested or verified. The comment does not provide a specific code suggestion or ask for a specific test to be written. Therefore, it should be removed.
2. pkg/query-service/app/dashboards/model.go:616
- Draft comment:
Renamed map keys ('widget_title' to 'widget_name' and 'dashboard_title' to 'dashboard_name'). Ensure that all API consumers and documentation are updated accordingly to prevent breaking changes. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_uXVuc9E3hXbmWe8m
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 56da1e7 in 1 minute and 45 seconds
More details
- Looked at
31lines of code in1files - Skipped
0files when reviewing. - Skipped posting
21drafted comments based on config settings.
1. pkg/query-service/app/clickhouseReader/reader.go:5620
- Draft comment:
Avoid forming SQL by concatenating untrusted strings (e.g. using strings.Join for fingerprints, ruleID, metric names). Use parameterized queries or bindings to mitigate SQL injection risks. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. pkg/query-service/app/clickhouseReader/reader.go:5694
- Draft comment:
Several queries use fmt.Sprintf to embed variables (e.g. rule_id, metric_name) directly. This can be error–prone and may lead to SQL injection if inputs aren’t sanitized. Prefer parameterized queries consistently. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. pkg/query-service/app/clickhouseReader/reader.go:5970
- Draft comment:
When dynamically building SQL in functions like GetAllMetricFilterAttributeKeys, the logic is complex. Consider refactoring into modular helper functions to simplify maintenance and improve readability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. pkg/query-service/app/clickhouseReader/reader.go:6058
- Draft comment:
Many functions have very long queries with nested subqueries. Consider breaking them into smaller, documented helper functions for clarity and easier testing. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. pkg/query-service/app/clickhouseReader/reader.go:6450
- Draft comment:
When unmarshalling JSON from DB row (e.g. in GetInspectMetrics), errors are logged and returned. Consider wrapping such errors to provide additional context. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
6. pkg/query-service/app/clickhouseReader/reader.go:6680
- Draft comment:
The use of reflection in readRow and readRowsForTimeSeriesResult may cause performance overhead. Verify that performance is acceptable or refactor to use statically typed scanning. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. pkg/query-service/app/clickhouseReader/reader.go:6500
- Draft comment:
Queries that use Sprintf with table names (e.g. in GetMetricsAllResourceAttributes) embed identifiers directly. Although table names are likely fixed, document and ensure these values are trusted. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. pkg/query-service/app/clickhouseReader/reader.go:6570
- Draft comment:
This file is extremely large and handles many responsibilities. Consider splitting the ClickHouseReader implementation into separate modules (e.g. metrics, logs, traces) to improve maintainability and easier global search. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. pkg/query-service/app/clickhouseReader/reader.go:5551
- Draft comment:
Avoid directly injecting traceID values via fmt.Sprintf; use parameterized queries instead to prevent potential SQL injection. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. pkg/query-service/app/clickhouseReader/reader.go:5500
- Draft comment:
Multiple queries are built using fmt.Sprintf with dynamic values. Consider consistently using parameter placeholders which improves security and maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. pkg/query-service/app/clickhouseReader/reader.go:1848
- Draft comment:
The TTL-setting functions spawn goroutines that log errors without propagating them; ensure robust error handling (or collect errors) in asynchronous operations. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. pkg/query-service/app/clickhouseReader/reader.go:4374
- Draft comment:
The readRowsForTimeSeriesResult function uses reflection to scan row values. This may impact performance and readability; consider using strongly typed structs or manual type switches when possible. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. pkg/query-service/app/clickhouseReader/reader.go:3500
- Draft comment:
Magic numbers like '1800' appear repeatedly. Define a named constant (e.g., bucketIntervalSeconds) to improve clarity and ease future changes. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. pkg/query-service/app/clickhouseReader/reader.go:5000
- Draft comment:
Enhance error handling by wrapping errors with additional context (e.g., using errors.Wrap) rather than returning generic error messages. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. pkg/query-service/app/clickhouseReader/reader.go:1
- Draft comment:
This file exceeds 6500 lines. Consider splitting the codebase into separate modules (e.g., for logs, metrics, traces, TTL handling) to improve maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
16. pkg/query-service/app/clickhouseReader/reader.go:6500
- Draft comment:
Dynamic query construction via string concatenation (e.g., in GetInspectMetricsFingerprints) can be error-prone. Consider using a SQL builder library to safely construct complex queries. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
17. pkg/query-service/app/clickhouseReader/reader.go:4071
- Draft comment:
Consider renaming functions like getPersonalisedError to getPersonalizedError for consistency with standard spelling conventions. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%
None
18. pkg/query-service/app/clickhouseReader/reader.go:5200
- Draft comment:
Identifiers such as rule_id are directly injected into SQL queries; parameterize these values to further mitigate SQL injection risks. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
19. pkg/query-service/app/clickhouseReader/reader.go:6000
- Draft comment:
Consider externalizing complex raw SQL queries from inline string concatenations to improve code readability and maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
20. pkg/query-service/app/clickhouseReader/reader.go:3447
- Draft comment:
Typographical error: The function call 'logs.CheckIfPrevousPaginateAndModifyOrder' contains a typo. Please change 'Prevous' to 'Previous'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
21. pkg/query-service/app/clickhouseReader/reader.go:5330
- Draft comment:
Typographical error: The type 'ReleStateItem' appears to be misspelled. It likely should be 'RuleStateItem' to stay consistent with the context of rule state history. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_yO89h7ZXUWhXqwHD
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
srikanthccv
reviewed
Mar 11, 2025
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on fb9b47e in 2 minutes and 27 seconds
More details
- Looked at
409lines of code in5files - Skipped
0files when reviewing. - Skipped posting
17drafted comments based on config settings.
1. pkg/query-service/utils/filter_conditions.go:14
- Draft comment:
Consider adding additional documentation/unit tests for BuildFilterConditions, especially clarifying the purpose and behavior of the optional skipKey parameter. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. pkg/query-service/utils/filter_conditions.go:51
- Draft comment:
Review the operator handling in buildSingleFilterCondition – ensure that numeric comparisons (if needed) are handled appropriately and that the string operators (like 'like' and 'match') are used consistently. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. pkg/query-service/utils/filter_conditions.go:109
- Draft comment:
Verify and document the time-range based table selection logic in WhichTSTableToUse to help future maintainers understand boundary conditions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. pkg/query-service/utils/filter_conditions.go:14
- Draft comment:
In BuildFilterConditions, each filter value is formatted via ClickHouseFormattedValue before being embedded into a condition string. Please confirm that this helper properly escapes user inputs to avoid SQL injection risks. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. pkg/query-service/utils/filter_conditions.go:51
- Draft comment:
In buildSingleFilterCondition, using fmt.Sprintf for query construction with user inputs may risk SQL injection. Consider using parameterized queries or enhanced escaping. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. pkg/query-service/utils/filter_conditions.go:109
- Draft comment:
WhichTSTableToUse uses manual time adjustments with modulo arithmetic to align the start time. Adding inline documentation explaining the rationale would improve maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. pkg/query-service/utils/filter_conditions.go:155
- Draft comment:
WhichAttributesTableToUse adjusts the start time to the nearest 6 hours; consider adding a comment to explain why only the start time is adjusted and not the end time. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment is asking for documentation, not pointing out a bug or suggesting a code improvement. The pattern of adjusting start times to boundaries is established elsewhere in the codebase. The function is small and follows an existing pattern. Adding a comment about why end time isn't adjusted would not materially improve the code.
The adjustment of start time but not end time could be confusing to future maintainers. The function's behavior differs from WhichTSTableToUse which has multiple cases and tables.
While documentation could help, the code follows an established pattern in the file. If the pattern needs documentation, it should be added at a higher level, not just for this specific function.
Delete the comment. It's requesting documentation for a pattern that's consistent throughout the file, and doesn't suggest any actual code improvements.
8. pkg/query-service/app/metricsexplorer/summary.go:164
- Draft comment:
In GetMetricsSummary, dashboard details are processed by marshalling then unmarshalling the data. Consider refactoring to avoid the double conversion for performance and clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. pkg/query-service/app/metricsexplorer/summary.go:137
- Draft comment:
The GetActiveTimeSeriesForMetricName call uses a hardcoded 120-minute window. Consider making this value configurable or document why 120 minutes is chosen. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. pkg/query-service/constants/constants.go:60
- Draft comment:
In GetAlertManagerApiPrefix, os.Getenv is called twice. Cache the result in a temporary variable to avoid redundant environment lookups. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. pkg/query-service/interfaces/interface.go:90
- Draft comment:
The Reader interface is very large and includes methods that are not strictly related to ClickHouse operations (e.g., QueryDashboardVars). Consider refactoring into separate interfaces or modules to improve maintainability and clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. pkg/query-service/app/clickhouseReader/reader.go:5749
- Draft comment:
Typographical error: The variable name 'atrributeValue' is misspelled. Please change it to 'attributeValue' for consistency and clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. pkg/query-service/app/clickhouseReader/reader.go:5447
- Draft comment:
Typographical error: The type 'ReleStateItem' appears to be misspelled. It likely should be 'RuleStateItem' to be consistent with the domain terminology. Please review and correct this if appropriate. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. pkg/query-service/app/metricsexplorer/summary.go:220
- Draft comment:
Typographical error: The constant 'TimeSeriesTeeMap' appears to be a typo. It likely should be 'TimeSeriesTreeMap' to match the naming convention (e.g., 'SamplesTreeMap'). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. pkg/query-service/app/metricsexplorer/summary.go:394
- Draft comment:
Typographical error: The function name 'getQueryRangeForRelateMetricsList' should likely be renamed to 'getQueryRangeForRelatedMetricsList' for consistency with the rest of the code. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
16. pkg/query-service/constants/constants.go:16
- Draft comment:
Typo in the comment for 'PrivateHostPort': the phrase "Address to server internal services" should be corrected to "Address to serve internal services". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
17. pkg/query-service/utils/filter_conditions.go:80
- Draft comment:
Typographical note: In the case for v3.FilterOperatorNotRegex (line 80), the string is formatted as 'not match(%s, %s)'. For consistency with similar functions (e.g., 'notLike(%s, %s)'), consider changing this to 'notMatch(%s, %s)'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_DexfxFuOHsnm5ufd
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
srikanthccv
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Added Inspect Metrics Api, it would help the user to get the educational info about different metrics aggregations.
Important
Introduces a new API for inspecting metrics, adding methods to
ClickHouseReader,SummaryService, andAPIHandler, with updates to constants and interfaces.GetInspectMetricsDataroute inhttp_handler.gofor inspecting metrics.GetInspectMetricsinSummaryServiceto handle inspect metrics requests.GetInspectMetricsandGetInspectMetricsFingerprintsmethods inClickHouseReader.GetMetricsAllResourceAttributesinClickHouseReaderto fetch resource attributes.GetAttributesForMetricNameto accept time range parameters.rows.Err()in multiple methods inreader.go.SIGNOZ_ATTRIBUTES_METADATA_TABLENAMEandSIGNOZ_ATTRIBUTES_METADATA_LOCAL_TABLENAMEtoconstants.go.Readerinterface ininterface.goto include new methods for inspect metrics.ParseInspectMetricsParamsinparser.goto validate time duration.GetDashboardsWithMetricNamesinmodel.go.This description was created by
for fb9b47e. It will automatically update as commits are pushed.